-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add OpenRouterModel as OpenAIChatModel subclass #3089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajac-zero Muchas gracias Anibal!
|
Buen día @DouweM, can you take a look when you get the chance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gracias!
It'd be interesting to add support for the WebSearchTool built-in tool as well, shouldn't be too complicated I think: https://openrouter.ai/docs/features/web-search
|
@ajac-zero We can also remove this comment from openai.py: pydantic-ai/pydantic_ai_slim/pydantic_ai/models/openai.py Lines 558 to 560 in c5b1495
|
Co-authored-by: Douwe Maan <[email protected]>
Co-authored-by: Douwe Maan <[email protected]>
|
Hi, just found this useful pr and I think top_k and other missing model config should be added to align with the Request Schema documented here https://openrouter.ai/docs/api-reference/overview. |
|
@ajac-zero Please have a look at the failing linting & coverage! |
|
@DouweM This part from pydantic-ai/pydantic_ai_slim/pydantic_ai/models/openai.py Lines 684 to 689 in 63d1b84
I fixed it by regexing the content afterward, but I don't like this solution very much. pydantic-ai/pydantic_ai_slim/pydantic_ai/models/openrouter.py Lines 474 to 477 in 63d1b84
I am hesitant on changing |
|
@ajac-zero Yep I think pulling this into a method that can be overridden in a subclass is a good idea. It shouldn't be specific for |
| maybe_event.part.id = 'content' | ||
| maybe_event.part.provider_name = self.provider_name | ||
| yield maybe_event | ||
| async def _validate_response(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other new methods need a return type hint
| By default, this is a no-op since `ChatCompletionChunk` is already validated. | ||
| """ | ||
| async for chunk in self._response: | ||
| yield chunk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just return self._response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it but no :( If we use return the method returns a coroutine instead, we have to use yield for it to behave as an async iterable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make the method sync though, and then return the coroutine, and I think it'll work
| @override | ||
| def _process_reasoning(self, response: chat.ChatCompletion) -> list[ThinkingPart]: | ||
| # We can cast with confidence because response was validated in `_validate_completion` | ||
| response = cast(OpenRouterChatCompletion, response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do an assert isinstance instead to raise an error if we somehow get here with an unexpected type?
| message = response.choices[0].message | ||
| items: list[ThinkingPart] = [] | ||
|
|
||
| if reasoning_details := message.reasoning_details: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we can entirely drop the .reasoning field processing we have in the superclass? Will OpenRouter always have reasoning_details as well?
| provider_details = super()._process_provider_details(response) | ||
|
|
||
| provider_details['downstream_provider'] = response.provider | ||
| provider_details['native_finish_reason'] = response.choices[0].native_finish_reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we 100% sure there's more than 1 choice at this point?
| if isinstance(item, TextPart): | ||
| texts.append(item.content) | ||
| elif isinstance(item, ThinkingPart): | ||
| if item.provider_name == self.system and isinstance(item, OpenRouterThinkingPart): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, this unfortunately won't work right :(
Hi! This pull request takes a shot at implementing a dedicated
OpenRouterModelmodel. Closes #2936.The differentiator for this PR is that this implementation minimizes code duplication as much as possible by delegating the main logic to
OpenAIChatModel, such that the new model class serves as a convenience layer for OpenRouter specific features.The main thinking behind this solution is that as long as the OpenRouter API is still fully accessible via the
openaipackage, it would be inefficient to reimplement the internal logic using this same package again. We can instead use hooks to achieve the requested features.I would like to get some thoughts on this implementation before starting to update the docs.
Addressed issues
Provider metadata can now be accessed via the 'downstream_provider' key in ModelMessage.provider_details:
The new
OpenRouterModelSettingsallows for the reasoning parameter by OpenRouter, the thinking can then be accessed as aThinkingPartin the model response:errorresponse from OpenRouter as exception instead of validation failure #2323. Closes OpenRouter uses non-compatible finish reason #2844These are dependent on some downstream logic from OpenRouter or their own downstream providers (that a response of type 'error' will have a >= 400 status code), but for most cases I would say it works as one would expect:
OpenRouterModel#1870 (comment)Add some additional type support to set the provider routing options from OpenRouter: